TCF experience design improvements #4222

galvana · 2023-10-03T23:50:24Z

Description Of Changes

Updating default fides.css and some component functionality to match our new designs https://www.figma.com/file/dmEwdK3xZwjKfGVQ9t66xe/Fides-v.2-Master-Working-Files?node-id=10555%3A602217&mode=dev. The designs should match for the most part with the following exceptions:

Responsive design was not considered as part of this iteration
Hide example / show example toggle was not implemented

Code Changes

fides.css and minor component changes

Steps to Confirm

Spin up fidesplus
Spin up the Privacy Center and navigate to http://localhost:3000/fides-js-demo.html

Pre-Merge Checklist

All CI Pipelines Succeeded
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md

cypress · 2023-10-05T22:06:03Z

Passing run #4523 ↗︎

0	4	0	0	0

Details:

Merge `db39bfe` into `3c767c4`...
Project: fides	Commit: `7309e222f2 ℹ️`
Status: Passed	Duration: 00:54 💡
Started: Oct 9, 2023 8:05 PM	Ended: Oct 9, 2023 8:06 PM

Review all test suite changes for PR #4222 ↗︎

clients/fides-js/rollup.config.mjs

galvana · 2023-10-06T17:59:57Z

clients/fides-js/src/components/ConsentBanner.tsx

+                <div id="fides-banner-title" className="fides-banner-title">
+                  {experience.title}
+                </div>
+                {showGpcBadge && bannerType !== "TCF" ? (


Passing in bannerType to hide the GpcBadge for the TCF banner/modal

hmm if its only purpose is whether or not to show the gpc badge, maybe instead of bannerType we make the prop hideGpc, then change above. I think that makes it a bit clearer for someone using this component exactly what that prop is going to do

const showGpcBadge = getConsentContext().globalPrivacyControl && !hideGpc;

actually maybe a better idea is to modify the function getGlobalPrivacyControl to query window.Fides.options.tcfEnabled. if it's enabled, return false. then I don't think gpc would ever render for TCF, and it'd be tied to gpc logic, as opposed to component logic, which is nice!

I like that, much cleaner!

clients/fides-js/src/components/ConsentBanner.tsx

clients/fides-js/src/components/ConsentModal.tsx

clients/fides-js/src/components/Overlay.tsx

clients/fides-js/src/components/fides.css

galvana · 2023-10-06T18:06:28Z

clients/fides-js/src/lib/a11y-dialog.tsx

+      const dialog = new A11yDialogLib(node);
+      dialog
+        .on("show", () => (document.documentElement.style.overflowY = "hidden"))
+        .on("hide", () => (document.documentElement.style.overflowY = ""));


Lightweight version of the scroll lock, the more robust version takes 1Kb https://a11y-dialog.netlify.app/advanced/scroll-lock/

allisonking

amazing job @galvana ! I'm going to play around some more locally, but wanted to leave you some comments first!

clients/fides-js/rollup.config.mjs

allisonking · 2023-10-06T18:15:42Z

clients/fides-js/src/components/ConsentBanner.tsx

  onVendorPageClick?: () => void;
+  buttonGroup: VNode;
+  bannerType?: String;


I didn't even know there was a String! looks like we should prefer string tho: https://stackoverflow.com/questions/14727044/what-is-the-difference-between-types-string-and-string

Suggested change

bannerType?: String;

bannerType?: string;

or even better, would be to restrict this to the strings it can be, i.e. "TCF"|"notices"

This ended up going away after refactoring the GPC check

clients/fides-js/src/components/ConsentBanner.tsx

allisonking · 2023-10-06T18:19:21Z

clients/fides-js/src/components/ConsentBanner.tsx

+                <div id="fides-banner-title" className="fides-banner-title">
+                  {experience.title}
+                </div>
+                {showGpcBadge && bannerType !== "TCF" ? (


hmm if its only purpose is whether or not to show the gpc badge, maybe instead of bannerType we make the prop hideGpc, then change above. I think that makes it a bit clearer for someone using this component exactly what that prop is going to do

const showGpcBadge = getConsentContext().globalPrivacyControl && !hideGpc;

allisonking · 2023-10-06T18:22:48Z

clients/fides-js/src/components/ConsentModal.tsx

 }: {
  attributes: Attributes;
  experience: ExperienceConfig;
  children: ComponentChildren;
  onVendorPageClick?: () => void;
+  modalType?: String;


same thing here—see my meandering comment thoughts about on bannerType

allisonking · 2023-10-06T18:40:37Z

clients/fides-js/src/components/fides.css

@@ -230,6 +258,7 @@ button.fides-banner-button.fides-banner-button-secondary {
  background: var(--fides-overlay-secondary-button-background-color);
  color: var(--fides-overlay-secondary-button-text-color);
  border: 1px solid var(--fides-overlay-primary-button-background-color);
+  border-radius: var(--fides-overlay-button-border-radius);


hm can this border-radius be defined on .fides-banner-button instead of on the two hover states? or does that not work since the border isn't defined until the hover state? (just trying to see if we can cut down on one line of CSS 😵 )

allisonking · 2023-10-06T18:41:48Z

clients/fides-js/src/components/fides.css

@@ -243,7 +272,9 @@ button.fides-banner-button.fides-banner-button-tertiary {
  color: var(--fides-overlay-link-font-color);
  text-decoration: underline;
  cursor: pointer;
-  line-height: 2em;
+  font-weight: 500;
+  font-size: var(--fides-overlay-font-size-body);


hmm should all the buttons be this size by default?

allisonking · 2023-10-06T18:44:18Z

clients/fides-js/src/components/fides.css

 }

 .fides-gpc-badge {
  text-transform: uppercase;
  padding: 0 4px;
  font-weight: 700;
-  border-radius: var(--fides-overlay-button-border-radius);
+  border-radius: 4px;


can this be

Suggested change

border-radius: 4px;

border-radius: var(--fides-overlay-badge-border-radius);

?

allisonking · 2023-10-06T18:48:32Z

clients/fides-js/src/components/ConsentBanner.tsx

+                <div id="fides-banner-title" className="fides-banner-title">
+                  {experience.title}
+                </div>
+                {showGpcBadge && bannerType !== "TCF" ? (


actually maybe a better idea is to modify the function getGlobalPrivacyControl to query window.Fides.options.tcfEnabled. if it's enabled, return false. then I don't think gpc would ever render for TCF, and it'd be tied to gpc logic, as opposed to component logic, which is nice!

clients/fides-js/src/components/ConsentBanner.tsx

clients/fides-js/src/components/ConsentButtons.tsx

allisonking · 2023-10-06T19:35:06Z

clients/fides-js/src/components/tcf/TcfOverlay.tsx

+                style={{
+                  display: "flex",
+                  justifyContent: "space-between",
+                  alignItems: "center",
+                  gridColumn: 1,
+                  gridRow: 2,


I don't think this styling is doing anything, is it? since there's only the one item inside

Good catch!

allisonking · 2023-10-06T19:38:11Z

clients/fides-js/src/components/notices/NoticeOverlay.tsx

+                style={{
+                  display: "flex",
+                  justifyContent: "space-between",
+                  alignItems: "center",
+                }}


same thing here—I don't think this is doing anything since there's only one child element? trying to save you some kb 😉

allisonking

okay @galvana I'm done reviewing for now! let me know what you think of the comments I left. none of them are really blocking, but I think they're worth considering. awesome job, should be ready to merge soon! ✨

clients/fides-js/src/components/tcf/TcfTabs.tsx

allisonking

amaaaaazing!!!

allisonking · 2023-10-10T13:47:47Z

clients/fides-js/src/lib/consent-context.ts

@@ -13,6 +13,10 @@ declare global {
 * having to modify the browser before the script runs.
 */
 const getGlobalPrivacyControl = (): boolean | undefined => {
+  if (window.Fides.options.tcfEnabled) {


let's update the docstring above to also mention how we don't use GPC with TCF

galvana added 2 commits October 3, 2023 16:48

First pass

c13775c

Merge branch 'main' into PROD-1084-TCF-experience-design-improvements

2057c1f

galvana linked an issue Oct 3, 2023 that may be closed by this pull request

TCF experience design improvements #4185

Closed

galvana added 2 commits October 5, 2023 09:47

Additional styling

ad84405

Matching more of the designs

0be3aea

galvana added 4 commits October 5, 2023 17:40

Adding class to banner button group

ae8fb9c

Adding middle button prop to button group

1c5badd

Preventing modal from closing when clicking outside

ff59616

Merge branch 'main' into PROD-1084-TCF-experience-design-improvements

6830081

galvana commented Oct 6, 2023

View reviewed changes

clients/fides-js/rollup.config.mjs Show resolved Hide resolved

galvana commented Oct 6, 2023

View reviewed changes

clients/fides-js/src/components/ConsentBanner.tsx Show resolved Hide resolved

galvana commented Oct 6, 2023

View reviewed changes

clients/fides-js/src/components/ConsentModal.tsx Show resolved Hide resolved

galvana commented Oct 6, 2023

View reviewed changes

clients/fides-js/src/components/Overlay.tsx Show resolved Hide resolved

galvana commented Oct 6, 2023

View reviewed changes

clients/fides-js/src/components/fides.css Show resolved Hide resolved

galvana commented Oct 6, 2023

View reviewed changes

galvana requested a review from allisonking October 6, 2023 18:06

galvana marked this pull request as ready for review October 6, 2023 18:06

galvana added 2 commits October 6, 2023 11:24

Lint fixes

d33f8ce

Running prettier

baf9545

allisonking reviewed Oct 6, 2023

View reviewed changes

galvana added 2 commits October 6, 2023 13:35

Differentiating between banner and modal title fonts

25211f2

Changes based on PR feedback

32aa418

galvana requested a review from allisonking October 6, 2023 22:19

galvana added 3 commits October 9, 2023 10:55

Merging in main

e689a4c

Moving all on off buttons

528788b

Running prettier

2803482

One more styling change

db39bfe

galvana commented Oct 9, 2023

View reviewed changes

clients/fides-js/src/components/tcf/TcfTabs.tsx Show resolved Hide resolved

allisonking approved these changes Oct 10, 2023

View reviewed changes

galvana added 2 commits October 10, 2023 08:55

Updating change log and docstring

b208e11

Merge branch 'main' into PROD-1084-TCF-experience-design-improvements

0d66d64

galvana merged commit e30c439 into main Oct 10, 2023
9 checks passed

galvana deleted the PROD-1084-TCF-experience-design-improvements branch October 10, 2023 15:57

galvana mentioned this pull request Oct 12, 2023

Fides-js: add link to privacy policy on banner #4229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TCF experience design improvements #4222

TCF experience design improvements #4222

galvana commented Oct 3, 2023 •

edited

Loading

cypress bot commented Oct 5, 2023 •

edited

Loading

galvana Oct 6, 2023

allisonking Oct 6, 2023

allisonking Oct 6, 2023

galvana Oct 6, 2023

galvana Oct 6, 2023

allisonking left a comment

allisonking Oct 6, 2023

allisonking Oct 6, 2023

galvana Oct 6, 2023

allisonking Oct 6, 2023

allisonking Oct 6, 2023

allisonking Oct 6, 2023

galvana Oct 9, 2023

allisonking Oct 6, 2023

allisonking Oct 6, 2023

allisonking Oct 6, 2023

allisonking Oct 6, 2023

galvana Oct 6, 2023

allisonking Oct 6, 2023

galvana Oct 6, 2023

allisonking left a comment

allisonking left a comment

allisonking Oct 10, 2023

	border-radius: 4px;
	border-radius: var(--fides-overlay-badge-border-radius);

TCF experience design improvements #4222

TCF experience design improvements #4222

Conversation

galvana commented Oct 3, 2023 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

cypress bot commented Oct 5, 2023 • edited Loading

Passing run #4523 ↗︎

Review all test suite changes for PR #4222 ↗︎

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonking left a comment

Choose a reason for hiding this comment

allisonking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galvana commented Oct 3, 2023 •

edited

Loading

cypress bot commented Oct 5, 2023 •

edited

Loading